Simplify NDCube.to_nddata #892
Conversation
| unit=True, | ||
| meta=True, | ||
| psf=True, | ||
| nddata_type=NDData, |
There was a problem hiding this comment.
My only remaining question is if we want to add a copy=False kwarg here which changes the copy behaviour to be copy by value rather that the default of copy-by-reference?
There was a problem hiding this comment.
I'm afraid that, as currently written, I don't see this as an improvement to what has already been merged for a few reaons.
First, we can't use bool to represent attributes that need to be copied because True is a valid user-defined value for mask.
Secondly, what we currently have allows users to convert to an NDCubeBase subclass while carrying over extra_coords and global_coords. I can imagine this being useful, especially if users want to change a value during the conversion, e.g. uncertainty.
Thirdly, if to_nddata is being used between NDCubeBase subclasses with additional attributes/kwargs, then this line that has been removed in this PR, captures those. By removing it, this code now unnecessarily would cause such cases to break. Alternatively, it unnecessarily requires maintainers of NDCubeBase subclasses to over-ride this method.
Additionally, I don't understand the depth of the objection to the to_nddata method. I agree that it's not the most elegant philosophically speaking. But as far as I can see, it is the most elegant practical solution to a problem that our users need solved.
| data=True, | ||
| wcs=True, | ||
| uncertainty=True, | ||
| mask=True, |
There was a problem hiding this comment.
We can't use mask=True here as True is a valid user input for mask.
There was a problem hiding this comment.
Oh, that's so freaking stupid.
There was a problem hiding this comment.
The solution to this is that the one user who wants to do this uses np.True_ rather than True to set the whole mask to be a single boolean.
| if key in inspect.signature(nddata_type).parameters.keys()} | ||
| **kwargs} | ||
| # If any are True then copy by reference | ||
| user_kwargs = {key: getattr(self, key) if value is True else value for key, value in user_kwargs.items()} |
There was a problem hiding this comment.
| user_kwargs = {key: getattr(self, key) if value is True else value for key, value in user_kwargs.items()} | |
| user_kwargs.update(kwargs) | |
| nddata_sig = inspect.signature(nddata_type).parameters.keys() | |
| user_kwargs = {key: value for key, value in user_kwargs.items() if key in nddata_sig and value is not COPY} | |
| all_kwargs = {key.strip("_"): value for key, value in self.__dict__.items() if key in nddata_sig} | |
| all_kwargs.update(user_kwargs) |
eugh, I still don't think breaking this truly weird case is worth the extra complexity.
It doesn't break this or any other use cases it just means you have to opt-into copying these things. See the test and the new example in the docstring.
see above.
I think we are conflating too many tasks into one method which makes the API and the implementation too complex both for us to maintain and for users to understand. I think we need to de-scope this thing. I'd almost rather just get rid of |
I know you're joking, but...no. 😆 I think the source of complexity is that we are trying to use the call signature as documentation. That's what the docstring is for. There are plenty of cases in Python of call signatures like
In the interests of getting this out to users quickly for the motivating use-case, I am happy to see the allowed For example: def to_nddata(self, nddata_type=NDData, copy=False, **kwargs):
"""
Constructs new type instance with the same attribute values as this `~ndcube.NDCube`.
Attribute values can be altered on the output object by setting a kwarg with the new
value, e.g. ``data=new_data``. Kwargs in addition to those listed below will be passed to
the constructor of ``nddata_type``.
Parameters
----------
nddata_type: `astropy.nddata.NDDataBase` subclass, optional
The type of the returned object. Only those from the `astropy.nddata`
subpackage are supported. Default=`astropy.nddata.NDData`.
copy: `bool`
Indicates whether values are fully copied or copied by reference.
Default=False => copied by reference.
data: array-like, optional
Data array of new instance. Default is to use data of this instance.
wcs: `astropy.wcs.wcsapi.BaseLowLevelWCS`, `astropy.wcs.wcsapi.BaseHighLevelWCS`, optional
WCS object of new instance. Default is to use data of this instance.
uncertainty: Any, optional
Uncertainy object of new instance. Default is to use data of this instance.
mask: Any, optional
Mask object of new instance. Default is to use data of this instance.
unit: Any, optional
Unit of new instance. Default is to use data of this instance.
meta: dict-like, optional
Metadata object of new instance. Default is to use data of this instance.
psf: Any, optional
PSF object of new instance. Default is to use data of this instance.
Returns
-------
new_nddata: (`astropy.nddata.NDData`, `astropy.nddata.NDDataRef`,
`astropy.nddata.NDDataArray`, `astropy.nddata.CCDData`)
The new instance of class given by ``nddata_type`` with the same values
as this `~ndcube.NDCube` instance, where supported, except for any alterations
specified by the kwargs.
Examples
--------
To create an `astropy.nddata.NDData` instance which is a copy of an `ndcube.NDCube`
(called ``cube``) without a WCS, do:
>>> nddata_without_coords = cube.to_nddata(wcs=None) # doctest: +SKIP
"""
# Check that nddata_type is supported.
supported_types = (astropy.nddata.NDData, astropy.nddata.NDDataRef,
astropy.nddata.NDDataArray, astropy.nddata.CCDData)
if not isinstance(nddata_type, supported_types):
raise TypeError(
f"For nddata_type expected one of: {supported_types}. Got {nddata_type}.")
# Gather kwargs from attributes of NDCube instance and update with user-supplied kwargs.
supported_inputs = inspect.signature(nddata_type).parameters.keys()
all_kwargs = {key.strip("_"): value for key, value in self.__dict__.items() if key in supported_inputs}
if copy:
all_kwargs = copy.deepcopy(all_kwargs)
all_kwargs.update(kwargs)
return nddata_type(**all_kwargs) |
2 participants
I couldn't take it.